Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Added new exception for nested fields to force it to fallback to legacy #205

Closed
wants to merge 1 commit into from

Conversation

GumpacG
Copy link

@GumpacG GumpacG commented Jan 14, 2023

Signed-off-by: Guian Gumpac [email protected]

Description

A new exception was made for nested fields that is thrown when the field is of type (ExprCoreTye)ARRAY. There is one old unit test and one old integration tests that are failing and to be investigated.

Issues Resolved

opensearch-project#1277

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov
Copy link

codecov bot commented Jan 14, 2023

Codecov Report

Merging #205 (dc17e9f) into integ-nested-fallback (9d4a841) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@                     Coverage Diff                     @@
##             integ-nested-fallback     #205      +/-   ##
===========================================================
- Coverage                    98.35%   98.32%   -0.04%     
- Complexity                    3609     3611       +2     
===========================================================
  Files                          344      345       +1     
  Lines                         8946     8952       +6     
  Branches                       569      571       +2     
===========================================================
+ Hits                          8799     8802       +3     
- Misses                         142      144       +2     
- Partials                         5        6       +1     
Flag Coverage Δ
sql-engine 98.32% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...a/org/opensearch/sql/analysis/TypeEnvironment.java 100.00% <100.00%> (ø)
...ch/sql/exception/UnsupportedV2NestedException.java 100.00% <100.00%> (ø)
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java 97.36% <0.00%> (-2.64%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -48,6 +50,12 @@ public TypeEnvironment(TypeEnvironment parent, SymbolTable symbolTable) {
@Override
public ExprType resolve(Symbol symbol) {
for (TypeEnvironment cur = this; cur != null; cur = cur.parent) {
if (!cur.symbolTable.lookup(symbol).isEmpty()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is already a fallback mechanism HERE. Could we alter this to work for nested with partiql syntax? At a quick glance I'm not sure why it's not working.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason the running SELECT message.info doesn't hit that code actually. Will investigate. Thanks!

@GabeFernandez310
Copy link

Looks fine to me so far (except for the failing checks)...
Looking at what's failing, I'm not sure if you're missing a test somewhere?
Execution failed for task ':core:jacocoTestCoverageVerification'.

@GumpacG
Copy link
Author

GumpacG commented Jan 16, 2023

Thanks! I will have a test for the exception being thrown.

if (!cur.symbolTable.lookup(symbol).isEmpty()
&& cur.symbolTable.lookup(symbol).get().equals(ExprCoreType.ARRAY)) {
throw new UnsupportedV2NestedException(
String.format("can't resolve %s type in the new engine", symbol));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention unsupported...

"%s type is unsupported in the new engine"

@GumpacG GumpacG closed this Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants